Skip to content

Conversation

@mkomet
Copy link
Contributor

@mkomet mkomet commented Aug 17, 2024

Add support for realloc (and realloc_aligned) into the multi heap lib, where the buffer sent in will either be reused (maybe shrinked), or enlarged by allocating on any of the matching heaps of the multi heap.

See issue:
#77174

@zephyrbot zephyrbot added the area: Base OS Base OS Library (lib/os) label Aug 17, 2024
@github-actions
Copy link

Hello @mkomet, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

@npitre
Copy link

npitre commented Aug 20, 2024

The logic is wrong. You shouldn't be using sys_heap_usable_size() to base
any decision on. That only gives you the actual allocation size which may
be larger than the originally requested size due to heap block granularity.
But it won't give you the size of a potentially free adjacent block into
which the existing allocation can be expanded.

Also, the align argument is dropped when sys_heap_realloc() is called
which has the potential to fail the allocation otherwise.

And... is there a reason why enlarging an allocation requiring that the
memory content be moved would not be done in the same heap if possible?

The code could be simplified to something like this:

    struct sys_multi_heap_rec *rec = sys_multi_heap_get_heap(mheap, ptr);
    void *new_ptr = sys_heap_aligned_realloc(rec->heap, ptr, align, bytes);

    if (new_ptr == NULL) {
        new_ptr = sys_multi_heap_aligned_alloc(mheap, cfg, align, bytes);
        if (new_ptr != NULL) {
            memcpy(new_ptr, ptr,
                   MIN(sys_heap_usable_size(rec->heap, ptr), bytes));
            sys_multi_heap_free(mheap, ptr);
        }
    }

    return new_ptr;

Add support for realloc (and realloc_aligned) into the multi heap lib,
where the buffer sent in will either be reused (maybe shrinked),
or enlarged by allocating on any of the matching heaps of the multi heap.

Signed-off-by: Meir Komet <[email protected]>
@mkomet mkomet force-pushed the multi-heap-realloc branch from c241349 to 866e6e6 Compare August 20, 2024 18:52
@mkomet
Copy link
Contributor Author

mkomet commented Aug 20, 2024

The logic is wrong. You shouldn't be using sys_heap_usable_size() to base any decision on. That only gives you the actual allocation size which may be larger than the originally requested size due to heap block granularity. But it won't give you the size of a potentially free adjacent block into which the existing allocation can be expanded.

Also, the align argument is dropped when sys_heap_realloc() is called which has the potential to fail the allocation otherwise.

And... is there a reason why enlarging an allocation requiring that the memory content be moved would not be done in the same heap if possible?

The code could be simplified to something like this:

    struct sys_multi_heap_rec *rec = sys_multi_heap_get_heap(mheap, ptr);
    void *new_ptr = sys_heap_aligned_realloc(rec->heap, ptr, align, bytes);

    if (new_ptr == NULL) {
        new_ptr = sys_multi_heap_aligned_alloc(mheap, cfg, align, bytes);
        if (new_ptr != NULL) {
            memcpy(new_ptr, ptr,
                   MIN(sys_heap_usable_size(rec->heap, ptr), bytes));
            sys_multi_heap_free(mheap, ptr);
        }
    }

    return new_ptr;

Duly, noted and I simplified the logic as you suggested.

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@mmahadevan108 mmahadevan108 added this to the v4.1.0 milestone Oct 31, 2024
@nashif nashif merged commit 5595f66 into zephyrproject-rtos:main Nov 16, 2024
1 check passed
@github-actions
Copy link

Hi @mkomet!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Base OS Base OS Library (lib/os) area: Kernel

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants